Fix: DocumentEditor open delay by deferring content load#477
Fix: DocumentEditor open delay by deferring content load#477
Conversation
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughInitial Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 10 seconds.Comment |
Greptile SummaryThis PR optimises Confidence Score: 4/5Safe to merge with minor style/robustness improvements No P0/P1 bugs found. Two P2 findings: autofocus being unnecessarily included in the dependency array (causes spurious applyContent calls after initial load) and the hasLoadedInitialContentRef not being reset when the editor instance is recreated. Core animation-deferral logic and rAF cancellation are correct. src/components/editor/DocumentEditor.tsx — review the effect dependency array and ref reset behaviour Important Files Changed
Reviews (1): Last reviewed commit: "Defer DocumentEditor content load to fix..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (rafA != null) cancelAnimationFrame(rafA); | ||
| if (rafB != null) cancelAnimationFrame(rafB); | ||
| }; | ||
| }, [editor, content, contentType, autofocus]); |
There was a problem hiding this comment.
autofocus in deps triggers unnecessary applyContent after initial load
autofocus is only consumed in the initial-load rAF branch. Once hasLoadedInitialContentRef.current is true, any change to autofocus re-runs the effect and calls applyContent() — which, for large documents, runs a full JSON.stringify comparison — but never actually re-focuses the editor. Consider reading autofocus through a ref so it can be omitted from the dependency array:
const autofocusRef = useRef(autofocus);
useEffect(() => { autofocusRef.current = autofocus; }, [autofocus]);
// then in the rAF callback:
if (autofocusRef.current) { editor.commands.focus("end"); }This eliminates spurious applyContent calls while keeping the focus behaviour correct.
| if (hasLoadedInitialContentRef.current) { | ||
| // External update (e.g., ZeroDB sync from another tab). Apply immediately. | ||
| applyContent(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
hasLoadedInitialContentRef not reset on editor recreation
hasLoadedInitialContentRef is a component-scoped useRef and persists for the lifetime of the component instance. If TipTap's useEditor ever recreates the editor object (e.g. because an extension config prop changes), the effect runs with hasLoadedInitialContentRef.current === true and applies content synchronously on the fresh editor. In most cases this is harmless, but it also means the deferred-paint optimisation is silently bypassed for any editor rebuild. Adding a reset when editor changes would make the behaviour explicit:
useEffect(() => {
hasLoadedInitialContentRef.current = false;
}, [editor]);
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Modifies the initialization and synchronization timing of a core UI component (DocumentEditor), which requires manual verification to ensure no UI flickering or race conditions.
The explicit editor.commands.focus("end") call was overriding Tiptap's
native autofocus behavior. Per Tiptap's resolveFocusPosition, autofocus:
true means "start" of doc — so the original (pre-defer) code placed the
cursor at start. Tiptap natively focuses the empty doc at position 0
via setTimeout(0) from mount(); after deferred setContent replaces the
doc, the cursor maps to position 0 of the new doc which is the start.
No explicit focus call needed.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: This modifies the initialization lifecycle and timing of a core component. Deferring content load via animation frames can introduce race conditions and requires human review.
The Mathematics extension's NodeView calls katex.render() synchronously
every time a math node mounts, with no memoization. Reopening the same
doc (or rendering duplicate expressions across docs) repays the full
parse + DOM-build cost.
Add a CachedMathematics drop-in replacement that wraps BlockMath and
InlineMath with NodeViews backed by a module-scoped LRU cache around
katex.renderToString. Cache key = `${latex}\0${JSON.stringify(opts)}`,
shared across all editor instances. Memory cost ~75KB at the 500-entry
cap. Visual output, classes, dataset attrs, and click handling are
identical to upstream.
Caveat: assumes katexOptions.macros is not used (\gdef would mutate the
shared macros object across renders). Documented in the file.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: This PR modifies the core initialization lifecycle of the DocumentEditor and introduces a custom caching extension for KaTeX, which requires human verification of potential side effects.
setContent uses tr.replaceWith over the full doc range, and
ProseMirror's selection mapping for a position inside a fully-replaced
range biases to the END of the replacement. So even though Tiptap's
native autofocus runs on the empty doc and lands at position 0, the
later setContent shifts the cursor to the doc end.
Explicitly call focus("start") (or setTextSelection(0) when autofocus
is false) after applyContent in the initial-load rAF to match the
original autofocus:true semantics — cursor at start, optionally
focused.
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/editor/DocumentEditor.tsx">
<violation number="1" location="src/components/editor/DocumentEditor.tsx:985">
P2: Autofocus restores the cursor to the start instead of the end after deferred initial content load.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // ProseMirror's selection mapping land at the END of the replacement. | ||
| // Move the cursor to the start to match Tiptap's native autofocus:true. | ||
| if (autofocusRef.current) { | ||
| editor.commands.focus("start"); |
There was a problem hiding this comment.
P2: Autofocus restores the cursor to the start instead of the end after deferred initial content load.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/editor/DocumentEditor.tsx, line 985:
<comment>Autofocus restores the cursor to the start instead of the end after deferred initial content load.</comment>
<file context>
@@ -976,6 +978,14 @@ export function DocumentEditor({
+ // ProseMirror's selection mapping land at the END of the replacement.
+ // Move the cursor to the start to match Tiptap's native autofocus:true.
+ if (autofocusRef.current) {
+ editor.commands.focus("start");
+ } else {
+ editor.commands.setTextSelection(0);
</file context>
Removing the CachedMathematics wrapper. The rAF-deferred setContent in DocumentEditor already solves the original "dialog feels delayed on open" complaint by unblocking the modal-open animation. The cache only helps repeat opens / cross-doc duplicate expressions, which is a marginal win for typical usage, and costs a custom NodeView that has to stay in sync with upstream BlockMath/InlineMath plus a footgun for future katexOptions.macros usage. Easier to add back later if perf telemetry actually justifies it. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
1 participant
Defer initial content load in DocumentEditor to avoid blocking the modal's open animation with synchronous markdown parsing and KaTeX rendering.
content/contentTypefromuseEditorinitial options so the editor mounts empty and paints immediately.requestAnimationFramefor the first content load to ensure it runs after the modal's open paint.autofocusafter deferred initial load to focus at end of content.Summary by CodeRabbit
Performance
Bug Fixes